Skip to content

Apply accessors with greatest visibility first - #220

Draft
kevinthegreat1 wants to merge 1 commit into
FabricMC:mainfrom
kevinthegreat1:fix-accessor-visibility
Draft

Apply accessors with greatest visibility first#220
kevinthegreat1 wants to merge 1 commit into
FabricMC:mainfrom
kevinthegreat1:fix-accessor-visibility

Conversation

@kevinthegreat1

Copy link
Copy Markdown

Fixes #172. I'm not sure how to test this in mixin tests due to each test being in one file, but I ran my accessor test suite on it, and it worked fine.

@LlamaLad7

LlamaLad7 commented Jul 11, 2026

Copy link
Copy Markdown
Member

I'm not sure how to test this in mixin tests due to each test being in one file

You should have multiple mixins with different priorities and have the low priority one have the highest accessor visibility

I ran my accessor test suite on it, and it worked fine

I'm not sure what tests those are but this PR only sorts accessor methods within each individual mixin which is not enough to solve the issue.

@LlamaLad7

Copy link
Copy Markdown
Member

Rather than sorting I think it would be better to have the merging logic take the max visibility of the new and existing methods

@kevinthegreat1

Copy link
Copy Markdown
Author

Yeah, this is broken, sorry about that.

@kevinthegreat1
kevinthegreat1 marked this pull request as draft July 11, 2026 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Accessor merging should use the widest visibility level of all accessors

2 participants